-
-
Notifications
You must be signed in to change notification settings - Fork 654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store state.accounts[].realm
as a WHATWG URL object.
#4235
Conversation
386383a
to
c00b9e6
Compare
c00b9e6
to
7f4aa27
Compare
Just resolved a conflict. This remains the blocker for #4230. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisbobbe ! I've now read through the first half or so of this branch:
3f9892d jest: Prepare for eslint-plugin-jest
upgrade.
1c2d730 deps: Add typescript
as dev dependency.
affc920 jest: Update all Jest-related dependencies to their latest.
10622c6 render test: Use representative realm
value for messageTypingAsHtml
.
ceedce9 url tests: Use representative realm
value.
ee2764b url: Make getFullUrl
use URL constructor.
20427e7 url [nfc]: Inline all uses of getFullUrl
and remove it.
26272d8 webAuth: Better compare web auth callback's realm
with current realm.
0e8820e store: Replace/revive URL instances.
and have just the comments below.
src/__tests__/sentry-test.js
Outdated
beforeEach(() => { | ||
// This Jest lint rule is slightly concerning; its doc [1] | ||
// claims that `expect` statements won't run if they're outside | ||
// a `test` or `it` block. Empirically, here, that isn't true -- | ||
// changing the below assertion to its negation causes quite | ||
// visible failure output from Jest. | ||
// | ||
// [1] https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md | ||
// eslint-disable-next-line jest/no-standalone-expect | ||
expect(isSentryActive()).toBeFalse(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just inlining this beforeEach
and afterEach
into each of the two test
blocks? That seems like it'll be simpler than this comment. 🙂
From the docs:
https://jestjs.io/docs/en/api#beforeeachfn-timeout
it seems like this isn't in line with the intended usage of these methods anyway -- they're meant to be for setting up and cleaning up state that the tests need to use.
src/__tests__/sentry-test.js
Outdated
test('breadcrumbs', () => { | ||
Sentry.addBreadcrumb({ | ||
message: 'test message', | ||
level: Sentry.Severity.Debug, | ||
}); | ||
expect(() => { | ||
Sentry.addBreadcrumb({ | ||
message: 'test message', | ||
level: Sentry.Severity.Debug, | ||
}); | ||
}).not.toThrow(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then also we can skip this diff hunk. I'm pretty sure the intent of this test isn't "call this method and make sure it doesn't throw", but "call this method and check that it doesn't cause Sentry to become active". Evidently that's not how you read it, though 🙂 , so making that explicit is another reason it'd be good to simply write those expect
calls inline within the test.
@@ -202,15 +202,14 @@ describe('fetchActions', () => { | |||
}; | |||
fetch.mockResponseSuccess(JSON.stringify(response)); | |||
|
|||
expect.assertions(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool -- this expect(…).rejects
API sure is nicer than the old one!
// | ||
// This Jest lint rule is slightly concerning; its doc [1] | ||
// claims that `expect` statements won't run if they're outside | ||
// a `test` or `it` block. Empirically, here, that isn't true -- | ||
// changing the below assertion to its negation causes quite | ||
// visible failure output from Jest. | ||
// | ||
// [1] https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md | ||
// eslint-disable-next-line jest/no-standalone-expect | ||
expect(heartbeat.isActive()).toBeFalse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This one is a lot like the sentry-test.js
case -- but the checks are a lot more code (vs. 1 simple line), so inlining them isn't so appealing.
If that were the only thing, then a good solution could be to put these all in a helper function and call that at the end of each test. But: looking closer at what's being checked here -- particularly the check I've highlighted -- it seems like the point of it isn't really so much to be part of the test, as it was in sentry-test.js, but to protect the tests from each other by ensuring they don't leave active heartbeats lying around. So from that perspective, it's most appropriate to have them here in afterEach
rather than invoked explicitly from each test.
So I investigated this rule a bit. Not only does its description empirically seem wrong about calling expect
inside afterEach
-- but I also tried putting expect(2 + 2).toBe(5)
(a) outside the afterEach
call, at the toplevel of the describe
block, and (b) at the toplevel of the whole module, and in both cases Jest gave an appropriate error. So I think the jest/no-standalone-expect
rule is just mistaken, and we should probably just disable it globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I did that testing at the tip of this branch, with its Jest upgrade. So there's a small possibility that this is behavior which has changed in Jest recently, and the rule used to make more sense than it does now.)
A few occurrences of these rules will newly be flagged with the upcoming `eslint-plugin-jest` upgrade: jest/expect-expect: https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md jest/no-try-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md jest/no-standalone-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. It did, however, lead to an improvement in `sentry-test`, where we move some `expect`s from `beforeEach` and `afterEach` into the tests themselves, since logically they're part of the tests themselves [1]. [1] zulip#4235 (comment)
7f4aa27
to
f0cbe6b
Compare
Thanks for the review! I've pushed changes to address your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good! Just the small comments below.
I still need to read the remaining commits in the branch; but for the commits mentioned above at #4235 (review) , feel free to merge after these comments. Or a prefix of them, if you'd rather e.g. save "store: Replace/revive URL instances." to go along with the subsequent commits that use it.
.eslintrc.yaml
Outdated
# | ||
# [1] https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md | ||
# [2] https://github.com/zulip/zulip-mobile/pull/4235#discussion_r489984717 | ||
jest/no-standalone-expect: off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapt to the first two and disable the third, which seems unhelpful,
as noted in code comments. It did, however, lead to an improvement
in `sentry-test`, where we move some `expect`s from `beforeEach` and
`afterEach` into the tests themselves, since logically they're part
of the tests themselves [1].
I do appreciate that these lint rules highlighted this improvement for us. But I think the credit can be assigned just as well to jest/expect-expect
, which we're keeping 🙂 . The two tests that were improved were tests that had no expect
calls of their own.
In fact I think it's not a coincidence that the tests that were improved were tests that would have been flagged by expect-expect
alone, while the tests that wouldn't have been improved were tests that that rule thought was fine and only no-standalone-expect
complained about. The fact that those Sentry tests had no expect
of their own is closely tied to the fact that they were relying on afterEach
to do the actual testing; and the fact that the heartbeat tests were more self-contained tests and their afterEach
was more about protecting the tests from each other is closely tied to the fact that those tests had their own expect
calls where they did their job as tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
src/start/webAuth.js
Outdated
const url = tryParseUrl(callbackUrl); | ||
|
||
if (!url) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for this sort of "check return value" pattern, no blank line -- the check and the line above it that produces the value are closely connected, so best to keep them visually together
A few occurrences of these rules will newly be flagged with the upcoming `eslint-plugin-jest` upgrade: jest/expect-expect: https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md jest/no-try-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md jest/no-standalone-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. See more discussion of that one on GitHub [1] [2]. [1] zulip#4235 (comment) [2] zulip#4235 (comment)
f0cbe6b
to
c2d3026
Compare
A few occurrences of these rules will newly be flagged with the upcoming `eslint-plugin-jest` upgrade: jest/expect-expect: https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md jest/no-try-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md jest/no-standalone-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. See more discussion of that one on GitHub [1] [2]. [1] #4235 (comment) [2] #4235 (comment)
OK, done, thanks! |
ef64362
to
24c51e5
Compare
OK, just resolved some conflicts. I also found a wrinkle and pushed a fix, but I feel slightly uneasy about it; maybe there's a better way? It's implemented in 8b96642 webview: Prepare to "revive" realm in and a small tweak in that same file ( 8c0cc3f accounts: Make |
Hmm, I see! Seems like one underlying awkward thing here is that the call site of export default (scrollMessageId: number | null, auth: Auth): string => `
<script>
// …
document.addEventListener('DOMContentLoaded', function() {
${compiledWebviewJs}
compiledWebviewJs.handleInitialLoad(
${JSON.stringify(Platform.OS)},
${JSON.stringify(scrollMessageId)},
${JSON.stringify(auth)}
);
});
</script>
`; ... But then when thinking about ways to address that, I find another underlying awkward thing: our So I don't have a great idea for how to arrange this so that the computer checks we haven't missed something, for when these types potentially change again in the future. Failing that, this seems like a fine way to handle the types as they currently are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Small comments below.
I went through the codebase with the search rg '[rR]ealm.toString'
to look for likely places to convert further toward URL objects, and only found a couple of items, mentioned below. Also rg 'new URL\(.*realm'
and didn't find anything.
On another side of things, I did a pair of searches for comparisons that potentially need toString()
added: rg -i 'realm.*[!=]=='
and rg -i '[!=]==.*realm'
. Everything looked to be covered.
src/webview/js/js.js
Outdated
const auth: Auth = { ...rawAuth, realm: rawAuth.realm }; | ||
// Since its version 5.x, the `react-native-webview` library dispatches our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: blank line to separate these
@@ -260,7 +260,7 @@ class AuthScreen extends PureComponent<Props> { | |||
id_token: credential.identityToken, | |||
}); | |||
|
|||
openLink(`${this.props.realm}/complete/apple/?${params}`); | |||
openLink(`${this.props.realm.toString()}/complete/apple/?${params}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably wants to be a URL
computation.
(That'd be a followup, another thing we can do to take advantage of having URL
objects everywhere; for this branch a TODO comment would be fine.)
isUrlOnRealm(url, realm) ? /^(\/#narrow|#narrow)/i.test(url.split(realm).pop()) : false; | ||
export const isInternalLink = (url: string, realm: URL): boolean => | ||
isUrlOnRealm(url, realm) | ||
? /^(\/#narrow|#narrow)/i.test(url.split(realm.toString()).pop()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit deserves a TODO comment about replacing it with URL
, similar to a couple of other such comments added in this branch.
In an upcoming commit, we'll change the `Auth` type to have a URL object for the realm. We call `JSON.stringify` on the arguments passed to `handleInitialLoad`. For the `realm` property, this means it'll be a string URL, not a URL object. Here, we pick it out in preparation for "reviving" it back into its URL form in the body of `handleInitialLoad`.
Like we did in bfe7949, for ZulipVersion instances. We don't store any of these in Redux yet, but we will soon.
Changing the `Auth` type implicitly changes the `Account` and `Identity` types, too. Done with an effort to minimize unnecessary follow-on changes in this commit. Here, we don't propagate the realm in its URL form as far toward the eventual consumers of the realm as we'll eventually want to; we'll do that in upcoming commits. Wherever two realm values are compared for equality with `===`, we make sure they are both stringified first. Flow didn't warn about these cases, so we caught them in test failures and by scanning through a full-project search for the term 'realm'.
As we'll do in some upcoming commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.
As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.
As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. `getAvatarUrl` itself will likely disappear soon, when we make the shell crunchier for handling `.avatar_url` on users, cross-realm bots, and messages. But the changes we make to its callers, here, will continue to be beneficial after that.
As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. Also rename `this.state.realm` to `this.state.realmInputValue`, to be more descriptive.
…mUrl`. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.
As in the preceding handful of commits, stringify the realm exactly where we need it as a string. These should be the last sites in which it's not totally clear that the realm is well-formed data.
24c51e5
to
05e24ee
Compare
Thanks for the review! Glad to catch those things. 🙂 I've just pushed my changes. |
In fact, this might not need another review; your suggestions are small and were easily addressed. I'll merge this in the next few minutes unless you say otherwise. 🙂 |
Prompted recently by a performance concern in #4230 (see discussion), this is also something we've been thinking of in the background as part of #4146.
It was refreshing to see a recent, relevant issue in Jest get resolved quite quickly, and (maybe even more refreshing!) to be able to just bump up a few minor versions to get the fix. It was so easy because of the recent jump in a few major Jest versions we did recently. 🎉